-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CONTINT-685] - initial tests for validation of OpenApi request #767
base: master
Are you sure you want to change the base?
[CONTINT-685] - initial tests for validation of OpenApi request #767
Conversation
.../hxi_connector/live_ingester/domain/usecase/e2e/repository/OpenApiRequestValidationTest.java
Fixed
Show fixed
Hide fixed
...ation-test/java/org/alfresco/hxi_connector/live_ingester/util/insight_api/RequestLoader.java
Fixed
Show fixed
Hide fixed
...ation-test/java/org/alfresco/hxi_connector/live_ingester/util/insight_api/RequestLoader.java
Fixed
Show fixed
Hide fixed
...ation-test/java/org/alfresco/hxi_connector/live_ingester/util/insight_api/RequestLoader.java
Fixed
Show fixed
Hide fixed
...ation-test/java/org/alfresco/hxi_connector/live_ingester/util/insight_api/RequestLoader.java
Fixed
Show fixed
Hide fixed
...ation-test/java/org/alfresco/hxi_connector/live_ingester/util/insight_api/RequestLoader.java
Fixed
Show fixed
Hide fixed
...ation-test/java/org/alfresco/hxi_connector/live_ingester/util/insight_api/RequestLoader.java
Fixed
Show fixed
Hide fixed
...ation-test/java/org/alfresco/hxi_connector/live_ingester/util/insight_api/RequestLoader.java
Fixed
Show fixed
Hide fixed
...ation-test/java/org/alfresco/hxi_connector/live_ingester/util/insight_api/RequestLoader.java
Fixed
Show fixed
Hide fixed
...ation-test/java/org/alfresco/hxi_connector/live_ingester/util/insight_api/RequestLoader.java
Fixed
Show fixed
Hide fixed
@@ -0,0 +1,39 @@ | |||
url: /v1/ingestion-events |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should go this path - I'm not in favor of adding another static (json or yaml) requests - I think we should introduce some Java DTOs and use them. With introduction of these DTOs and new Mappers we could get rid of UpdateNodeEventSerializer
, DeleteNodeEventSerializer
and some static requests from our tests - WDYT @tpage-alfresco and @Pawel-608 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is just to extract the static json from e.g. CreateRequestIntegrationTest
and put it in a file where two tests can access it. I think what you're suggesting is to combine the tests so that we validate the output from the Connector directly against the remote schema, but I'm concerned that if the test starts failing then we won't know if it's due to our code changes or changes in the remote schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally I think we want to validate the exact output of the Connector (rather than just its validity), because otherwise we would pass tests by e.g. only producing delete requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I get your point - the idea is to perform validation in two steps:
- verify if connector output meats "our requirements",
- verify if "our requirements" comply with OpenAPI spec
One downside what I see here is: maintain complexity - if someone will forget to update this static yaml request we might end up in situation where Connector will be sending not valid requests and we might notice it quite late
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - I agree. It's important that all static requests are also included in e2e tests so that we know they can be produced by the application. I can't think of a neat way to verify that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a suggestion of how we could ensure we test all requests in the folder against the schema here: #767 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, that we could leave CreateRequestIntegrationTest
and others usecase.e2e
tests as they are and introduce new test verifying if our code complies with OpenAPI spec. If only this new one test would fail, it would mean, that the spec has changed. While when more tests will start to fail, then someone messed something up in the code.
Our Endpoint-to-Endpoint tests are very handy and bring a lot of value presenting in a simple way what will be the outcome for different repo events, so we shouldn't change them.
.../hxi_connector/live_ingester/domain/usecase/e2e/repository/OpenApiRequestValidationTest.java
Fixed
Show fixed
Hide fixed
.../hxi_connector/live_ingester/domain/usecase/e2e/repository/OpenApiRequestValidationTest.java
Outdated
Show resolved
Hide resolved
.../hxi_connector/live_ingester/domain/usecase/e2e/repository/OpenApiRequestValidationTest.java
Show resolved
Hide resolved
Request request = makeRequest(hxInsightRequest); | ||
|
||
assertThat(classUnderTest.validateRequest(request).getMessages()).isEmpty(); | ||
validatePropertiesField(hxInsightRequest.body(), propertiesSchema); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are very close to a generic test here. If we add in a check based on whether the request contains a properties
field then we can do something like:
for (requestFile : expected-hxinsight-requests folder)
{
HxInsightRequest hxInsightRequest = RequestLoader.load(requestFile);
Request request = makeRequest(hxInsightRequest);
assertThat(classUnderTest.validateRequest(request).getMessages()).isEmpty();
if (hxInsightRequest.hasField("properties")
{
validatePropertiesField(hxInsightRequest.body(), propertiesSchema);
}
}
Then we never need to update this file and we automatically check any new requests stored in the expected requests folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be done in separate pr
@@ -0,0 +1,14 @@ | |||
url: /v1/ingestion-events |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this method of testing to be valid we need to ensure that we're also checking the application can output these requests. We are doing this for create-document-request.yml
in CreateRequestIntegrationTest.java
, but I don't think we're doing it for the other three yml files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated testUpdateRequest() in UpdateRequestIntegrationTest.java class with "/expected-hxinsight-requests/update-document-request.yml". In UpdateRequestIntegrationTest.java there are test cases which use different request body - we do not use those in OpenApi tests. DeleteRequestIntegrationTest.java class was updated with "/expected-hxinsight-requests/delete-document-request.yml". There is no body for PresignedUrls case so no need for update there.
@@ -0,0 +1,38 @@ | |||
url: /v1/ingestion-events |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Files could be a bit different organized, like:
resources
|_ rest
|_ hxinsight
|_ requests
|_ create-document.yml
|_ create-document-without-file.yml
.
.
.
|_ responses
|_ sth.yml
WDYT @azakrzewski-hy ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change implemented
.../hxi_connector/live_ingester/domain/usecase/e2e/repository/OpenApiRequestValidationTest.java
Fixed
Show fixed
Hide fixed
.../hxi_connector/live_ingester/domain/usecase/e2e/repository/OpenApiRequestValidationTest.java
Fixed
Show fixed
Hide fixed
.../hxi_connector/live_ingester/domain/usecase/e2e/repository/OpenApiRequestValidationTest.java
Fixed
Show fixed
Hide fixed
.../hxi_connector/live_ingester/domain/usecase/e2e/repository/OpenApiRequestValidationTest.java
Fixed
Show fixed
Hide fixed
I've decided to not implement tests with invalid request as those are covered by other tests in this project. Messages triggered by OpenApi specification based on invalid requests are not produced by our project but by external library |
CONTINT-685
Initial tests for OpenApi request validation.
This PR does not contain final version of the work needed for the ticket.
If you have any advice / idea what could be the best way to cover the scope just let me know in commands.
Test is not finished yet:
testRequestToIngestionEvents()
it fails as string is not successfully validated agains OpenApi specification
expectedBody string is exactly the same as in CreateRequestIntegrationTest where we use it as the reference body